-
Notifications
You must be signed in to change notification settings - Fork 278
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor(cmd-api-server): pull OAuth2 endpoint scopes from openapi.json #3463
base: main
Are you sure you want to change the base?
refactor(cmd-api-server): pull OAuth2 endpoint scopes from openapi.json #3463
Conversation
5d2cd8e
to
3321ed3
Compare
} | ||
|
||
describe("cmd-api-server:getOpenApiSpecV1Endpoint", () => { | ||
const logLevel: LogLevelDesc = "TRACE"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change log level to info
, dont use trace
for tests in CI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@outSH Got it, just finished updating it.
3321ed3
to
f44cf0c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, Thank you!
it("HTTP - returns the OpenAPI spec .json document of the API server itself", async () => { | ||
const res1Promise = apiClient.getOpenApiSpecV1(); | ||
await expect(res1Promise).resolves.toHaveProperty("data.openapi"); | ||
const res1 = await res1Promise; | ||
expect(res1.status).toEqual(200); | ||
expect(res1.data).toBeTruthy(); | ||
|
||
console.log("Response data type:", typeof res1.data); | ||
console.log("Response data:", res1.data); | ||
|
||
let openApiSpec; | ||
try { | ||
openApiSpec = | ||
typeof res1.data === "string" ? JSON.parse(res1.data) : res1.data; | ||
} catch (error) { | ||
throw new Error(`Failed to parse OpenAPI spec: ${error.message}`); | ||
} | ||
|
||
expect(openApiSpec).toHaveProperty("components"); | ||
expect(openApiSpec.components).toHaveProperty("securitySchemes"); | ||
|
||
const securitySchemes = openApiSpec.components.securitySchemes; | ||
expect(securitySchemes).toBeObject(); | ||
|
||
const expectedScopes: IExpectedScopes = { | ||
"read:health": "Read health information", | ||
"read:metrics": "Read metrics information", | ||
"read:spec": "Read OpenAPI specification", | ||
}; | ||
|
||
const securitySchemeNames = Object.keys(securitySchemes); | ||
|
||
securitySchemeNames.forEach((schemeName) => { | ||
const scheme = securitySchemes[schemeName]; | ||
expect(scheme).toHaveProperty("flows"); | ||
const flows = scheme.flows; | ||
expect(flows).toHaveProperty("authorizationCode"); | ||
const scopes = flows.authorizationCode.scopes as IExpectedScopes; | ||
|
||
Object.keys(expectedScopes).forEach((scope) => { | ||
expect(scopes).toHaveProperty(scope); | ||
expect(scopes[scope]).toEqual(expectedScopes[scope]); | ||
}); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aldousalvarez This verifies that the OpenAPI .json file has the security scheme declared. It does not verify that those are being respected by the endpoint implementations themselves. Please update the endpoints touched by this diff to make use of the declared scopes and then write 2 more test cases for each endpoint in addition this one I'm commenting on. One of them should be the positive test case where with a valid JWT the API client can execute the request and with an invalid JWT the request execution fails with an HTTP 401 unauthorized error.
Note: by test cases I just mean another it("does something...", async () => {...})
block not a separate file.
f44cf0c
to
25c1eba
Compare
Hello @petermetz, Already done the requested changes, re-requested for your review. Thank you |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aldousalvarez Looking much better now but it's still missing the crucial negative test case for missing or incorrect scopes, please see my comment above.
expect(res1.status).toEqual(200); | ||
expect(res1.data).toBeTruthy(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aldousalvarez In this test case it is supposed to fail because you are executing with a JWT that does not have the scope declared (so the JWT has a valid signature but it does not declare the correct role based access control and therefore should not work)
7aeb0e1
to
dade3eb
Compare
Primary Changes ---------------- 1. added OAuth2 security endpoints scopes to openapi.json 2. added a test to make sure if the scopes are indeed getting pulled from the spec file Fixes hyperledger#2693 Signed-off-by: aldousalvarez <aldousss.alvarez@gmail.com>
dade3eb
to
4c58d17
Compare
Commit to be reviewed
refactor(cmd-api-server): pull OAuth2 endpoint scopes from openapi.json
Fixes #2693
Pull Request Requirements
upstream/main
branch and squashed into single commit to help maintainers review it more efficient and to avoid spaghetti git commit graphs that obfuscate which commit did exactly what change, when and, why.-s
flag when usinggit commit
command. You may refer to this link for more information.Character Limit
A Must Read for Beginners
For rebasing and squashing, here's a must read guide for beginners.